-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
time: delay the cancellation of timers #7467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
c2d5790
to
d04c22f
Compare
} | ||
|
||
#[tokio::test] | ||
async fn reset_later_after_slot_starts() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
This test was removed because it was testing the behavior of tokio::runtime::time::entry::reset
, which is removed in this PR.
} | ||
|
||
#[tokio::test] | ||
async fn reset_earlier_after_slot_starts() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
This test was removed because it was testing the behavior of tokio::runtime::time::entry::reset
, which is removed in this PR.
sleep(ms(20)).await; | ||
|
||
assert!(queue.is_woken()); | ||
assert_ready_some!(poll!(queue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
queue.reset_at
resets inner Sleep
, however, the new implementation will drop the inner timer and create a new one. So the waker will not be called, we have to poll manually.
sleep(ms(20)).await; | ||
|
||
assert!(queue.is_woken()); | ||
assert_ready_some!(poll!(queue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
queue.reset_at
resets inner Sleep
, however, the new implementation will drop the inner timer and create a new one. So the waker will not be called, we have to poll manually.
695eea8
to
e9255ee
Compare
feature = "signal", | ||
feature = "time", | ||
))] | ||
pub(crate) use wake_list::WakeList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
We no long use WakeList
in the time subsystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for taking long between reviewes, but this is a really large PR and I find it difficult to review. Here are a few things I noticed while looking at it, but I still don't feel like I have an overview.
/// True if the driver is being shutdown. | ||
is_shutdown: AtomicBool, | ||
|
||
// When `true`, a call to `park_timeout` should immediately return and time | ||
// should not advance. One reason for this to be `true` is if the task | ||
// passed to `Runtime::block_on` called `task::yield_now()`. | ||
// | ||
// While it may look racy, it only has any effect when the clock is paused | ||
// and pausing the clock is restricted to a single-threaded runtime. | ||
#[cfg(feature = "test-util")] | ||
did_wake: AtomicBool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving these atomics into their own Arc
is best avoided. That's an allocation per integer, and they may be stored far away from each other hurting cache locality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me revisit the shutdown process and relevant dataflows, will come back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved, details are attached to https://gist.github.com/ADD-SP/4037e25256b8dc8a4956962415de2356#file-d-propagate-shutdown-notification-md.
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
drop(lock); | ||
|
||
waker_list.wake_all(); | ||
pub(crate) struct Context2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Bad name, feel free to suggest a new one.
Review guide
Design document
See https://gist.github.com/ADD-SP/4037e25256b8dc8a4956962415de2356.
Benchmarks
See https://gist.github.com/ADD-SP/4037e25256b8dc8a4956962415de2356.